Skip to content

Fix derivation test build and UnmarshalEspressoTransaction Panics#397

Merged
shenkeyao merged 4 commits intocelo-integration-rebase-16.2from
keyao/fix-unmarshal
Apr 8, 2026
Merged

Fix derivation test build and UnmarshalEspressoTransaction Panics#397
shenkeyao merged 4 commits intocelo-integration-rebase-16.2from
keyao/fix-unmarshal

Conversation

@shenkeyao
Copy link
Copy Markdown
Member

@shenkeyao shenkeyao commented Apr 7, 2026 •
edited
Loading

Closes https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213948846870077?focus=true.
Closes https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213749992420791?focus=true.

This PR:

  • Fixes the build for op-node/rollup/derive.
  • Adds a length check for the signature to avoid panics on the transaction unmarshalling.
  • Adds a unit test for this fix.

How to test this PR:

  • Run go test -run '^TestUnmarshalEspressoTransactionTooShort$' ./op-node/rollup/derive/ and verify the test passes.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 7, 2026 •
edited
Loading

Claude finished @shenkeyao's task in 1m 14s —— View job

Code Review: Fix derivation test build and UnmarshalEspressoTransaction Panics

  • Gather context and read changed files
  • Review code quality and correctness
  • Check test coverage
  • Post review feedback

Overall: This PR looks good. The changes are small, focused, and correct. A few observations:

espresso_batch.go — Length check (the main fix)

The guard len(data) < crypto.SignatureLength correctly prevents the slice panic on line 103. The error message is clear and includes both the actual and required lengths. This is the right place for this validation since all callers funnel through UnmarshalEspressoTransaction.

blob_data_source_test.go — Build fix

The added false parameter to txmgr.MakeSidecar is consistent with all other call sites in the repo. Straightforward build fix.

espresso_batch_test.go — New test

Good test cases covering nil, empty, and just-under-threshold inputs. Left one minor inline comment about using require.Error instead of t.Errorf for consistency with the rest of the file.

You might also consider adding an edge case for data of exactly crypto.SignatureLength bytes (65 bytes) — this passes the length check but produces an empty batchData, which would fail at rlp.DecodeBytes. Testing this would confirm the function handles the "valid signature length but no payload" case gracefully without panicking.

claude[bot]
claude Bot reviewed Apr 7, 2026
View reviewed changes
Comment thread op-node/rollup/derive/espresso_batch_test.go Outdated
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a length check in UnmarshalEspressoTransaction to prevent potential panics when processing short transaction data. It also includes a corresponding unit test to verify this behavior and updates a test call to txmgr.MakeSidecar to match a signature change. I have no feedback to provide.

@shenkeyao @claude
Update op-node/rollup/derive/espresso_batch_test.go
9bc82ed 
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@philippecamacho
Copy link
Copy Markdown
Collaborator

philippecamacho commented Apr 8, 2026 •
edited
Loading

go test -run '^TestUnmarshalEspressoTransactionTooShort$' ./op-node/rollup/derive/

github.com/ethereum-optimism/optimism/op-node/rollup/derive
op-node/rollup/derive/espresso_batch_test.go:98:1: expected declaration, found '}'
FAIL github.com/ethereum-optimism/optimism/op-node/rollup/derive [setup failed]
FAIL

@shenkeyao
Copy link
Copy Markdown
Member Author

shenkeyao commented Apr 8, 2026

go test -run '^TestUnmarshalEspressoTransactionTooShort$' ./op-node/rollup/derive/

github.com/ethereum-optimism/optimism/op-node/rollup/derive op-node/rollup/derive/espresso_batch_test.go:98:1: expected declaration, found '}' FAIL github.com/ethereum-optimism/optimism/op-node/rollup/derive [setup failed] FAIL

@philippecamacho The test should pass now! I committed Gemini's review suggestion directly, which caused the previous failure.

@philippecamacho
Copy link
Copy Markdown
Collaborator

philippecamacho commented Apr 8, 2026

go test -run '^TestUnmarshalEspressoTransactionTooShort$' ./op-node/rollup/derive/

Yes, the test is passing now.

philippecamacho
philippecamacho approved these changes Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@philippecamacho philippecamacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shenkeyao shenkeyao merged commit f48ed43 into celo-integration-rebase-16.2 Apr 8, 2026
41 checks passed
@shenkeyao shenkeyao deleted the keyao/fix-unmarshal branch April 8, 2026 21:47
QuentinI pushed a commit that referenced this pull request Apr 9, 2026
@shenkeyao @claude @QuentinI
Fix derivation test build and UnmarshalEspressoTransaction Panics (#397)
31ad93a 
* Fix unmarshal panic

* Fix the build and add a test

* Update op-node/rollup/derive/espresso_batch_test.go

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

* Fix syntax

---------

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments

@philippecamacho philippecamacho philippecamacho approved these changes

@QuentinI QuentinI Awaiting requested review from QuentinI

@jjeangal jjeangal Awaiting requested review from jjeangal

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@shenkeyao @philippecamacho